-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leaks in some cache maps #2101
Conversation
Oh I forgot that |
Just add a comment about it (after you update the code) so that we can change it when we update the minimum Go version 🙂 |
…utine, added cache.NotifyCanvasRefresh, fix the way textures are freed, fix memory leak on window.Close
@andydotxyz @Jacalz it is not finished yet, but I would like to know your thoughts/suggestions about the current changes :) Basically, in the latest changes I have removed the go routine used to clean the cache in favor of using the paint events as the ticker. Some of my doubts are:
|
|
Indeed it is faster. Current PR use both of them (destroy on refresh and destroy when expired). However destroy on refresh will not destroy objects that are out of scope (because they no longer belonged to the object tree).
Hmm for primitives I mean the objects created with
Textures are created and destroyed really fast. The destroy action will delete immediately the texture from the textures cache map but not from the canvases map (we would need to wait them to expire to release that memory). So if we change the map data to:
So canvases map will not longer hold texture objects, and by doing so, we would just need to delete the texture from the textures cache map. Then |
Good point!
I don't think this is wise from what I have read - the texture cache refers to the current rendering of an object. If this object is changed and |
On yes, you are right, I missed that fact. Thanks :) |
…o theme changes and remove 10ms delay on every paint event
@andydotxyz I have moved the svg_cache to the cache package and removed the 10 ms delay in mobile driver. Could you a do quick review in |
@andydotxyz Ok this is almost done. Desktop side is ready (maybe better naming for the added cache functions is needed), however on mobile side I think a refactorization is needed specially to rid off the minSizeCache and use renderCacheNode-canvasesCache solution used in desktop (or viceversa, I am not sure what is the better solution, maybe was it discussed before?) So should I add TODO comments and leave this PR as it is now? Or should this PR wait until mobile driver is refactored (mainly just add the renderCacheNode and queueEvent method)? |
It would make it a lot easier to apply this in a shared-code manner if the mobile driver had the equivalent of 7461574 applied. I am nervous that the drivers are moving further apart instead of coming together, so let's not make the differences and performance gap greater yet. |
@andydotxyz Totally agreed. So should I open another PR as the initial step to try sharing code between the two drivers? Then this PR should wait until that PR is landed. Is that ok? |
If you're OK doing that I think it sounds like a great idea |
Yes, it is :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good. Just have a few suggestions for changes :)
Co-authored-by: Jacob <Jacalz@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. It is certainly a good improvement. I checked the memory usage before and after and and it certainly does seem to be much better at releasing memory. I still see the memory slowly climbing without me doing anything inside the application, but I guess that's another issue :)
Hmm the memory (after some time) should be constant (without increasing) 🤔. Could you share the code you are using to test this? I think I should add some test cases, I will try to add them later today. |
I was just using fyne_demo inside the branch that this PR is based on (on Linux). I think it started at 22Mb and then it slowly climbed to 29Mb. I don't know if this was before or after the time it should have stabilised in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, but it's not quite in-keeping with our code style.
You've included a lot of comments, some that are blocks to separate methods, and some that are telling what a line of code does - both seem superfluous.
If code needs a comment then it generally should be more cleanly written.
Additionally cache/base.go
adds a lot of new code but I don't think that any tests cover it.
internal/cache/base.go
Outdated
cleanTaskInterval = cacheDuration / 2 | ||
|
||
canvasRefreshCh = make(chan struct{}, 1) | ||
expiredObjects = make([]fyne.CanvasObject, 0, 50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be global, it is only used internally in two methods, both of which append and then clean out in a single run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it global only to avoid having to recreate the slice on every clean task, in that way the same slice is reused on every task without the need to allocate memory again. Should I make it local?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it global requires locking, surely being able to avoid that outweighs any time required in allocating memory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not require locking because cache.Clean won't run asynchronously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andydotxyz Any comment about this?
Hmm in fyne_demo, is a bit difficult to test this, because if you go to the screens that already have memory leaks, then the memory will never be stable |
Yes, you are right, I will try to add them later today. |
@andydotxyz I have worked in some of your suggestions, waiting for your review now. If everything goes fine, I will start writing the tests. |
…() for glfw driver as suggested in PR review
Looking good thanks. If tests can be added things should be good. |
Great :), I'll try to add them tonight. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are most likely more things we can do to improve the memory usage but this is a great step in the right direction. Nice work! 👍
Yes, nice work thanks. |
Description:
Potentially fix #348, #735, #2069.
Checklist:
CallUpdate mobile driver, canvas and windows to have the same desktop changes.cache.Clean
on mobile too.